Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend Operators #27

Merged
merged 29 commits into from
Aug 7, 2021
Merged

Extend Operators #27

merged 29 commits into from
Aug 7, 2021

Conversation

gxavier38
Copy link
Contributor

@gxavier38 gxavier38 commented Jun 22, 2021

I'm submitting a pull request with my work on PySNARK from the past few weeks.
I've mainly been extending operators, adding tests, and reformatting code according to PEP8.
I also implemented the Poseidon hash in PySNARK for my own use. Seems useful to have it in this repo.

There are some things I need help with. Firstly, I'm not entirely sure I have all the constraints implemented correctly. I might be missing a few, which might cause security issues.
Secondly, I added a CircleCI configuration file to for automatic testing in case it's useful for you. However, the qaptools and libsnark tests fail because I didn't set them up correctly. Since I haven't used those before, I don't exactly know how to set them up for CircleCI.

Sorry for the huge pull request. The changes were all interconnected so it didn't make sense to submit them individually.

Let me know which changes you'd like to have and which you don't.

Thanks!

Changes

Operators

  • Implemented unimplemented operators
  • Extended existing operators to work for more data types

For example:

  • LinComb.__pow__ by LinComb,
  • LinComb.__mod__ by integer that is not a power of 2,
  • LinComb.__mod__ by LinComb,
  • LinCombFxp.__divmod__, etc.

See tests/ folder for examples

Booleans

  • Added LinCombBool for boolean values with logical operators as discussed in Fix some operators in pysnark.runtime #18
  • Changed LinComb operators (e.g. __and__, __or__, __xor__) to be bitwise operations
  • Made if_then_else use LinCombBool

Code Quality

  • Added tests for LinComb, LinCombFxp, and LinCombBool
  • Added CircleCI script

Extensions

  • Added Poseidon hash gadget

To Do

  • Some LinCombFxp operators still aren't implemented, e.g. __pow__ by a LinCombFxp
  • Check constraints. Some constraints may be missing, which might affect security
  • Fix CircleCI script for libsnark and qaptools
  • Update examples to work with new changes

Resolves #23 and resolves #25

pysnark/runtime.py Outdated Show resolved Hide resolved
pysnark/runtime.py Outdated Show resolved Hide resolved
pysnark/runtime.py Outdated Show resolved Hide resolved
from .branching import if_then_else

# Limit exponent to prevent Python crashing with bus error
if other.value.bit_length() > 5:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if this would be somehow configurable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be a good way to handle it? Another global variable? An extra parameter?

What if we update each multiplicand with multiplicand.value = multiplicand.value % backend.get_modulus()? Then, the maximum exponent would be limited only by bitlength. That would make the output LinComb's value and __repr__ wrong, but would increase performance and shouldn't affect the actual circuit.

It might also be worth thinking about how to handle LinComb.value when it overflows. Should we leave the value as is or wrap around? If we were to wrap the value around (e.g. in __init__), it might easily solve these issues.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, doing multiplicand.value = multiplicand.value % backend.get_modulus() definitely makes sense. Of course other.to_bits still requires a bit length, but I guess we can just use the default bit length and if the user wants to have a lower bit length (because of efficiency) he can just temporarily change it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And indeed, it is a good point about the overflows. Originally the code used to just always reduce modulo backend.get_modulus(). But later my idea was to keep stuff completely independent from the modulus wherever possible. Because who knows, one day there may be backend that just works over the integers and doesn't use a modulus at all. In fact, the only place where get_modulus is now used is in the hash code.

Maybe instead of using backend.get_modulus(), the backends should instead support a backend.reduce(val) function that performs modular reduction...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I just reduce using backend.get_modulus() for now?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's just keep it like that at least for pow. It doesn't make much sense to do a pow that has an overflow, so in practice it shouldn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the change, but oblivious pow now costs 7 * bitlength + 1 constraints. Is that acceptable?

@meilof
Copy link
Owner

meilof commented Jun 27, 2021

Thanks a lot for your work! I will go through it over the next days but it may take some time..

@gxavier38
Copy link
Contributor Author

Thanks for your help! There's no rush.
I'll respond to your comments as they come.

pysnark/fixedpoint.py Outdated Show resolved Hide resolved
return LinCombFxp((self.lc * other) / (1 << resolution))
if isinstance(other, LinCombFxp):
return LinCombFxp((self.lc * other.lc) / (1 << resolution))
return NotImplemented
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work. It will const bit_length constraints due to the right-shift (at least I assume (1<<resolution) is implemented with a right shift in runtime.py? otherwise it is more costly). The previous code did two assert_positive's of size equal to the resolution. So it seems that depending the resolution and the bit length either of the two could be more efficient.. There would also be differences in the sizes of the inputs that they would accept.. I would have to think a bit more about which version would be preferable in which cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. LinComb.__rshift__ uses a division right now instead of to_bits. I'll have to change that and change the fixed point code to use >> (1 << resolution).

Could you explain the constraints you had before? Are they still required here?

diff = (1<<LinCombFxp.res)*ret.lc-self.lc*other.lc
(diff + (1<<LinCombFxp.res)).assert_positive(LinCombFxp.res+1)
((1<<LinCombFxp.res) - diff).assert_positive(LinCombFxp.res+1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I did some digging and it turns out that LinCombFxp.__mul__ costs only 1 constraint, even with the division. I traced it to LinComb.__truediv__ costing 0 constraints to divide by a public integer, and (1 << resolution) being a public integer.

Could you check that LinComb.__truediv__ is correct?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LinComb.truediv as it is now implements proper division, meaning it only works if the division does not have a remainder. So you can divide 8 by 4 (and so do a right-shift by two), but not 9 because this division has remainder one. So you cannot use truediv to implement a right-shift, it should be floordiv. I think for a right-shift there is really no other way than doing a bit decomposition one way or the other

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
I'll implement it using floordiv for now. Using a direct rshift with bit decomposition is cheaper, but we easily run into trouble because of to_bits since the numbers are often larger than 2 ** bitlength.

pysnark/boolean.py Outdated Show resolved Hide resolved
pysnark/boolean.py Outdated Show resolved Hide resolved
pysnark/runtime.py Outdated Show resolved Hide resolved
pysnark/runtime.py Outdated Show resolved Hide resolved
pysnark/runtime.py Outdated Show resolved Hide resolved
@meilof meilof merged commit ca79e6e into meilof:master Aug 7, 2021
@meilof
Copy link
Owner

meilof commented Aug 7, 2021

I finally went through the pull request in full and merged it into master. Thanks a lot for your work! I also added your name to the acknowledgements section in the README, I hope that is OK?

I still found a few small things where I think efficiency could be improved, so hopefully I or you can still fix them in the future. And hopefully I can find time to do some sanity checks to see if everything does what we expect. For future reference, here's some notes I made:

  • some of the LinCombBool operations can be implemented more efficiently
    since we know that it stores bits, e.g., a<=b <=> not(a=1 and b=0)
  • implement __pow__ using square-and-multiply
  • runtime __mod__, __floordiv__ with a power of two can be more efficient

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementing Exponentiation Mod operator
2 participants